This fail-fast guard only covers one of the two doors into generation, and the capability layer disagrees with it.

1. generate_image() is unguarded. sd_version_is_wan() was (correctly) extended to include VERSION_ABOT_WORLD, so ABot travels through all the shared Wan latent/VAE/sampling paths used by both generate_video() and generate_image()/generate_image_internal(). But the rejection was added only here. A caller that loads an ABot GGUF and calls generate_image() runs the bidirectional/one-shot Wan recipe on a causal model with no check at all — silently corrupted output, or a raw GGML_ASSERT abort if it hits a shape assumption that was never true for ABot. That's exactly the hazard this guard exists to prevent.

2. The capability query now lies. sd_version_supports_video_generation() (line 3104) is ... || sd_version_is_wan(version) || ..., so it now reports ABot as video-capable. The example server's /vid_gen route (examples/server/routes_sdcpp.cpp) uses that query to reject unsupported models cleanly before reaching the engine — it will instead wave ABot through to this return false, bypassing the intended clean-rejection path.

Suggested fix: reject in one shared choke point instead of per-entrypoint, and keep the capability query honest:

• exclude ABot from sd_version_supports_video_generation() (and any image-capability query), e.g. ... && !sd_version_is_abot_world(version);
• move the actual rejection to a point both entrypoints pass through (GenerationRequest / SamplePlan::resolve()), or reject at new_sd_ctx() load time when the version is VERSION_ABOT_WORLD and no causal-session API was requested.

Loading still works; generation is then refused everywhere with the same message, and the server pre-screen agrees with the engine.

Both points confirmed and fixed in 23d8fd5 — thanks, this was a real gap.

• Shared choke point: the rejection moved into GenerationRequest::resolve() (same pattern as validate_ideogram4_uncond_model), so generate_image() and generate_video() are both covered by one check; the per-entrypoint guard here was removed. Message now names both entrypoints.
• Honest capability queries: sd_version_supports_video_generation() and sd_version_supports_image_generation() both return false for ABot (image could not simply stay !video, or it would have flipped to true).
• script/validate_abot_world.sh grew an img_gen lane asserting the image path rejects too; verified locally against the ABot Q8_0 GGUF (detection + full load unchanged, both batch paths reject, stock-Wan lane unaffected).

On load-time rejection: kept loading permitted intentionally — downstream (tetherto/qvac#3352) exercises load on GPU CI ahead of the causal session API, and the capability queries now give front-ends the clean pre-screen.

These writes commit the session state before the initial KV capture below has succeeded. If INIT_CAPTURE fails, step() returns failure with non-empty history but no base cache. A subsequent call sees first == false, takes kv_fast, and reaches the GGML_ASSERT(k_all != nullptr && v_all != nullptr) in the cache provider. Decode and deferred-append failures can likewise leave history, ring metadata, and backend cache tensors at different logical block numbers.

Because backend cache writes may already be partially applied when a graph fails, rollback does not look reliable. I suggest making the session explicitly fail-stop. For example, add terminal state to AbotWalkSession:

bool failed = false;

bool fail_session(const char* reason) {
    if (!failed) {
        LOG_ERROR("abot session entered terminal failure state: %s; recreate the session", reason);
    }
    failed = true;
    kv_append_pending = false;
    return false;
}

Then reject every later entry before it can inspect history or use the cache:

bool step_latents(/* ... */) {
    if (failed) {
        LOG_ERROR("abot session: step called after terminal failure; recreate the session");
        return false;
    }
    // ...
}

bool step(/* ... */) {
    if (failed) {
        LOG_ERROR("abot session: step called after terminal failure; recreate the session");
        return false;
    }
    // ...
}

Stateful failures should poison the session rather than returning a reusable object, for example:

if (flow.empty()) {
    return fail_session("KV initial capture failed");
}

if (px.empty()) {
    return fail_session("frame decode failed");
}

The same should cover denoising and append failures: even before history is committed, denoising has advanced the RNG, so retrying would no longer reproduce the same block. The C API documentation should state that a failed sd_abot_session_step() requires freeing and recreating the session.

Thank you — issue confirmed: a failed INIT_CAPTURE returned with history already committed but no base cache, so the next step took the kv_fast path into the cache provider GGML_ASSERT; and with the RNG advanced no failure is retryable anyway. Fix is the fail-stop you suggested: terminal failed flag + fail_session(), entry guards in step()/step_latents(), poisoning at every stateful failure point (denoise forward, INIT_CAPTURE, append, decode), and the terminal-failure contract documented on sd_abot_session_step(). Submitted in 81a0f02 — success paths verified bit-identical (2-block KV walk, block-0 frames at infinite PSNR vs pre-change).

This mutates kv_ring_next before forward_step_kv() has succeeded, and kv_ring_abs is also updated before the flow.empty() check below. Even with a terminal-failure flag, it would be safer to commit host-side bookkeeping only after successful execution. For example:

int next_ring_slot = kv_ring_next;
for (int f = 0; f < Fb; ++f) {
    frames.push_back(kv_append_frames[f].data());
    fts.push_back(cfg.context_noise_t);
    abs_ids.push_back(total - Fb + f);
    write_slots.push_back(next_ring_slot);
    next_ring_slot = (next_ring_slot + 1) % AbotWorldRunner::kv_ring_slots;
}

auto ring_before = kv_ring_abs;
auto flow = runner->forward_step_kv(/* ... */);
if (flow.empty()) {
    return fail_session("KV append failed");
}

kv_ring_next = next_ring_slot;
for (int f = 0; f < Fb; ++f) {
    kv_ring_abs[write_slots[f]] = abs_ids[f];
}
kv_append_pending = false;
return true;

kv_append_pending is currently cleared at function entry as well; that should move to the successful commit section. This cannot roll back backend writes, which is why the terminal flag is still needed, but it prevents the host metadata from advertising an append that never completed.

Thank you — confirmed, all three instances: kv_ring_next advanced during setup, kv_ring_abs written before the flow.empty() check, and kv_append_pending cleared at entry (so a failed append would be silently skipped on the next call). Fix is exactly the shape you sketched: slots computed in a local next_ring_slot/write_slots, the append graph reads the untouched pre-append kv_ring_abs, and kv_ring_next/kv_ring_abs/kv_append_pending commit only after successful execution (with the terminal flag from the other thread covering the backend side). Submitted in 81a0f02.

This one is worth a second look, less for the style than for the failure mode.

The comment directly above states the path produces wrong output for any block with history (cosine ~0.2 vs goldens). Shipping that as a process-wide env var means:

• it's set once for the process, with no per-session or per-call scoping — a front-end that runs an ABot session alongside other work can't enable it for one and not the other;
• there's no signal in the output that it was on. LOG_INFO at :970 is the only trace, so a stray ABOT_FLASH_ATTN=1 in a shell profile or a CI env produces silently degraded walks that look plausible;
• it can't be exercised or asserted against from a test that doesn't shell out.

The codebase already models this exact concept as a params field — sd_ctx_params_t.flash_attn / diffusion_flash_attn (include/stable-diffusion.h:218-219). That struct isn't reachable here (the session is deliberately standalone, no sd_ctx_t), so the equivalent would be a bool flash_attn; on sd_abot_session_params_t, defaulted off in sd_abot_session_params_init.

Given it's documented as broken, the other reasonable option is to not expose it at all right now — put it behind a build-time define, or drop it until the history-mask handling is fixed. An opt-in debug path is fine; an opt-in debug path reachable by an environment variable in a shipped shared library is the part I'd push back on.

Thank you, issue confirmed — a known-broken path one ambient env var away is not acceptable in a shipped library. Fix: ABOT_FLASH_ATTN is no longer read in production builds; the flash path is gated behind the SD_ABOT_FLASH_ATTN_DEBUG compile define (with a LOG_WARN when enabled there). The abot-parity/abot-walk harnesses keep their env toggles — they exist to debug exactly this. Submitted in 52b231e.

Same suggestion as the flash-attn toggle above, though with less urgency since this path is validated rather than known-broken: this reads naturally as bool kv_cache; on sd_abot_session_params_t next to the local_attn_size / num_frame_per_block fields it interacts with.

The interaction is the actual argument for moving it. kv_ring_slots is a compile-time 5 (:551) while local_attn_size is caller-settable, and the two are only consistent at the default 8/3 — but the value that decides whether that matters arrives through a completely different channel (env) than the value that breaks it (params). With both on the same struct, sd_abot_session_new can cross-validate them in one place and fail fast, instead of the mismatch only showing up as quietly-wrong attention several blocks into a walk.

Also worth noting kv_decode_overlap_safe is derived here from backend identity — that's good, and it's the kind of thing that's much easier to reason about when the enabling flag is a visible field rather than ambient process state.

Thank you, confirmed — fix is bool kv_cache on sd_abot_session_params_t (env var no longer read; the API is unreleased so no compat impact), and with both values finally in one struct, session load now cross-validates the window against the compiled ring and fails fast. Byte-identical KV walk vs the old env path (21/21 frames). Submitted in 52b231e.

ABOT_PROF is the most defensible of the three env vars — it's pure diagnostics with no effect on computed output, it's read once into a static const, and this codebase does have precedent for that shape (SD_CPU_ONLY, SD_VK_DEVICE, GGML_ROPE_FLUX_DISABLE).

No objection to leaving it as-is. Flagging only for consistency if flash_attn / kv_cache do move onto sd_abot_session_params_t — at that point a bool profile; alongside them costs nothing and keeps all three discoverable from the header. Your call; this one I'd genuinely not block on.

Thank you — added bool profile alongside the other two for discoverability; ABOT_PROF stays as a fallback for field debugging, per your non-blocking note. Submitted in 52b231e.

This struct is where I'd expect the three walk toggles to live. It already carries the same class of configuration — thread count, seed, block size, attention window, param offload — so bool flash_attn; and bool kv_cache; here (defaulted off in sd_abot_session_params_init) would make the full behaviour of a session readable from the header, settable per-session, and reachable from language bindings that don't control the process environment.

As it stands, two settings that materially change both output correctness and memory profile are invisible at this API boundary: a binding author reading this header has no way to discover that ABOT_FLASH_ATTN or ABOT_KV_CACHE exist, or that one of them is documented as producing wrong results.

Separate small thing on the neighbouring field: local_attn_size at :520 documents <= 0 as "scene/deploy default", but nothing carries a deploy value — no field in the scene pack or the model holds one — so 0 always resolves to the compiled-in 8, which abot_world.hpp:57 calls the test config (vs. 21 deployed). Either the comment should name the real fallback or the value needs to come from somewhere.

Thank you, confirmed on both points. kv_cache and profile now live on this struct (flash-attn is compile-gated as known-broken, so deliberately not exposed here), making the toggles discoverable and per-session. The local_attn_size comment now names the real fallback: compiled default 8, with a note that the upstream deployed config uses 21 and that kv_cache validates the window at load. Submitted in 52b231e.

First, the question that sets the severity of this and the next three comments: is a scene pack ever externally sourced?

• Always generated locally by sd_abot_scene_create and never moved between machines → these are robustness nits, fine to defer.
• Ever downloaded, shared, or served from a registry → this parser is an untrusted-input surface reachable straight from the public API (sd_abot_session_new → scene.load(scene_path), :993) and should be hardened before shipping.

The format looks built to be shareable — portable safetensors, the artifact a session consumes — which is why I'm asking rather than assuming.

Here, hlen comes from the first 8 bytes of the file and goes straight into std::string header(hlen, '\0') with no upper bound. Sixteen bytes of input (hlen = 0xFFFFFFFFFFFFFFFF) forces a multi-exabyte allocation. Real safetensors loaders cap header size for exactly this reason.

Also on the next line: f.read(header.data(), hlen) doesn't check gcount(), so a truncated file leaves header silently zero-padded and parsing proceeds over the padding rather than failing.

Worth noting the blast radius regardless of provenance: these entry points are inside the header's extern "C" block and there's no try/catch anywhere in the chain, so bad_alloc / length_error propagates across a C-linkage boundary. For an embedding host — the Node/Bare addon path — that's a process kill, not an error return. A try/catch at the C API boundary is worth having on its own merits.

Thank you — and to answer the question: yes, packs are externally sourced in practice (the upstream HF repo ships downloadable reference packs, and packs move between machines in our validation flows), so the untrusted case applies. The parser now treats every field as untrusted: header length capped at 16 MB with gcount checks, and the whole chain sits behind an exception barrier at the extern "C" boundary (session_new/step, scene_create) so bad_alloc and friends degrade to nullptr/false instead of terminating the embedding host. Validated with 8 hostile packs — all clean rejections, no crashes. Submitted in 52b231e.

sp, sb and se are never tested against npos (same applies to op / ob at :123-125).

If a pack contains the tensor name but no "shape" key after it, sp == npos; header.find('[', npos) legally returns npos, so sb == se == SIZE_MAX. The loop on the next line then starts at sb + 1, which wraps to 0, with the bound q < se effectively unbounded. From there the only thing stopping it is a character match against header[q] — and std::string::operator[] is unchecked, so once q passes header.size() this is an out-of-bounds heap read that runs until it happens to hit a byte satisfying the exit condition. strtoll(header.c_str() + q, ...) at :120 reads past the end too.

This is reachable with a merely truncated pack, not just a crafted one, because of the unchecked gcount() noted on :104.

Fix is mechanical: check each find() result against npos and return false before doing any arithmetic derived from it.

Thank you, confirmed (including the npos+1 wraparound on the data_offsets find). Fix: every find() is npos-checked and a tensor with a missing shape/data_offsets fails the load with a named error. Submitted in 52b231e.

n (computed at :126-127) is the raw product of file-supplied dimensions with no overflow check, and out.resize(ne) is sized directly from it with no cap.

Two distinct failure modes:

1. Large-but-not-overflowing dims (say [2000000000, 2000000000]) → multi-exabyte allocation attempt → bad_alloc → uncaught across the C boundary, as on :104.
2. Dims whose product overflows int64_t wrap to an unrelated small or negative value, so the tensor's declared shape and its actually-allocated buffer disagree — downstream indexing uses the un-wrapped shape metadata. Classic size-confusion setup.

Note tensor_numel (src/tensor.hpp:47-60) only rejects negative dims, and does so by throwing std::invalid_argument — which is the same uncaught-exception path again.

Scene-pack tensors are small fixed-purpose objects (prompt embeds, one first-frame latent, a handful of ref slots), so a generous hard cap on element count plus an explicit overflow check on the dimension product would cost nothing and close both.

Thank you, confirmed — both failure modes. Fix: dims must be positive, the element product is overflow-safe and capped at 256M elements (1 GiB, far above the largest legitimate tensor), and each tensor's data range is checked against the actual file size before reading. Submitted in 52b231e.

sh[2] is indexed with no check that torch_shape actually has rank 3 — and sh was populated purely from whatever digits appeared between [ and ] in the header. Same pattern at :150 (sh[4], sh[3], needs rank 5) and :164-165 (sh[5], needs rank 6).

A pack declaring prompt_embeds with a rank-1 or rank-2 shape is perfectly well-formed JSON and gets an out-of-bounds std::vector read here. This is the cheapest of the four to trigger — no truncation or crafted npos needed, just a wrong-rank shape.

Suggest validating rank per named tensor right after the fetch call (3 / 5 / 6 respectively) and failing closed before touching any sh[i].

Thank you, confirmed. Fix: an explicit rank + leading-singleton layout check per named tensor (prompt_embeds rank 3, first_frame_latents rank 5, ref_latents rank 6) before any fixed-position indexing or reinterpreting resize; ref_mask must also match ref_latents' slot count. A rank-2 prompt_embeds pack now fails cleanly. Submitted in 52b231e.

This constant is derived from the test config, but local_attn_size and num_frame_per_block are both caller-settable via sd_abot_session_params_t and are wired in at src/stable-diffusion.cpp:5825-5830 with no cross-validation against it. kv_ring_abs (:879) is a fixed std::array of this size.

The mismatch: build_mask_kv:599 computes the window bound as hi - cfg.local_attn_size, but only kv_ring_slots history columns physically exist in the ring. The non-KV path scales correctly — history_keep at :1072 derives keep from cfg.local_attn_size — so the recompute path honours the configured window and the KV path cannot.

Concrete scenario: a front-end sets local_attn_size = 21 (the value the comment at :57 calls the deployed default, vs. the 8 it calls the test config) and enables ABOT_KV_CACHE=1 for the documented speedup. After 5 finalized blocks the ring wraps, and frames still inside the intended 21-frame window aren't masked out — they're structurally absent as K/V columns. From there the DiT attends a ~5-frame window instead of 21. No assert, no log, no error; the walk keeps producing plausible frames with quietly wrong temporal context, diverging from both the recompute path and the reference.

script/validate_abot_world.sh and all three examples/abot-* harnesses build AbotWorldConfig cfg; at its defaults, so this combination is uncovered.

Either:

• fail fast in AbotWalkSession::load() when kv_enabled && (cfg.local_attn_size - cfg.num_frame_per_block > kv_ring_slots), naming both values — turns silent wrong output into a startup error; or
• size the ring at runtime from cfg (std::vector rather than std::array, and the per-layer abot.kv.l{i}.{k,v}.r{s} tensors with it), mirroring what the non-KV path already does.

Thank you, confirmed — this was the highest-value catch of the pass. Fix is your fail-fast option, made possible by moving kv_cache onto the params struct: session load rejects kv_cache when local_attn_size - num_frame_per_block exceeds the compiled ring, with an error naming both values. Verified: kv_cache + local_attn_size=21 now fails at load; the default 8/3 KV walk is byte-identical to before (21/21 frames). The runtime-sized ring stays on the follow-up list (QVAC-21981). Submitted in 52b231e.

This is only ever appended to (:1165) and never trimmed, so it grows for the lifetime of the session.

history_keep bounds what enters the compute graph (the kept subset at :1072-1088), not this backing store — so the bounded-memory work in this PR fixes VRAM growth but leaves the host-side buffer with the shape it set out to eliminate. Nothing beyond max(keep, decode_overlap) + Fb frames from the tail is ever read again, yet all of it stays resident.

At the documented default latent size a frame is 52*30*48 floats ≈ 292 KB, so at Fb = 3 that's ~880 KB per block, uncapped — a couple of GB per hour of continuous walking, on an API whose whole purpose is open-ended interactive sessions. Memory-constrained targets will hit it first.

Flagging that this is not a one-liner: history.size() doubles as the absolute-frame counter at :909, :1039, :1108 and :1208, so trimming needs a logical frame-index offset threaded through those sites to keep abs_ids correct after eviction. A bounded ring sized to max(keep, decode_overlap) + Fb plus an explicit frames_emitted counter would do it.

Thank you, confirmed — and agreed it is not a one-line fix (history.size() doubles as the absolute frame counter at four sites). Deferred as a follow-up per your note, tracked under QVAC-21981: bounded ring/deque sized to max(keep, decode_overlap) + Fb plus an explicit frames_emitted counter.

This tensor is rebuilt on every denoise step, and it's very large.

c_unsh = cfg.act_in_dim * ds * ds = 8192 (:463), so at 832x480 with F_vis ≈ 27 this is 52 * 30 * 8192 * 27 ≈ 345M floats — ≈1.4 GB allocated, filled and host→device copied per step, ~5.5 GB per block across the 4 steps in cfg.denoise_steps. And fill_act_plane (:437-443) fills each channel plane with a single constant, so that 1.4 GB is carrying 8 bits of keyboard state.

The rebuild is avoidable regardless of size: inside the denoise loop at :1090, kept, action_mask and frame_abs_ids are all invariant — only xt and the timestep change. Yet build_pe / build_mask run unconditionally at :502-503 (and :665-670 on the KV path), as does this fill.

Two independent wins:

• Hoist build_pe / build_mask / fill_act_plane / rows out of the s loop in step_latents, compute once per block, pass the already-uploaded backend tensors in, and rebuild only x_all / tvec. Straightforward ~4x on this cost.
• Don't materialize the action planes at all — whatever the adapter conv needs, a broadcast of 8 bits shouldn't require a multi-GB dense tensor per step.

The KV path is much cheaper (scoped to the current block: mask ~26 MB, action ~153 MB) but has the same redundant 4x rebuild. Minor related nit: zero_k / zero_v (:659-664) are allocated and zero-filled every KV step even once the ring is fully populated and no slot has ring_abs[s] < 0.

Thank you, confirmed — the per-step rebuild of block-invariant buffers (incl. the ~1.4 GB action tensor on the recompute path) is real. Deferred as a follow-up per your note, tracked under QVAC-21981: hoist pe/mask/action out of the denoise loop (~4x on that cost), then stop materializing the broadcast planes entirely. The zero_k/zero_v nit rides along.